fix: [M3-6889] CreateCluster styles #9835
Conversation
| fontWeight: 600, | ||
| letterSpacing: '0.25px', | ||
| lineHeight: '1.33rem', | ||
| margin: 0, |
There was a problem hiding this comment.
Those were the offenders. Those styles have been there since day one this feature was created.
A good reminder to be careful with nested global selectors.
abailly-akamai
requested review from
jdamore-linode and
carrillo-erik
and removed request for
a team
There was a problem hiding this comment.
Looks good, thanks @abailly-akamai!
Checked out the LKE cluster create form in Chrome, Safari, and Firefox and everything looked as I expected to, and I didn't notice any regressions. Also checked smaller viewports.
You can also disregard the Cypress failure; it was an HTTP 504 during a before hook.
|
Great job! |
| label="Cluster Label" | ||
| value={label || ''} | ||
| /> | ||
| <Divider sx={{ marginTop: 4 }} /> |
There was a problem hiding this comment.
Quick question, I've noticed in other places of the code when we pass a styling value via sx prop like in this fashion we at time use included the css unit like this sx= {{ marginTop: '20px' }}. If I append the css unit to this line it changes the UI.
Have we established a preferred method to do this?
There was a problem hiding this comment.
Good question. SX is nice because the spacing it tied to our design system. (aka theme.spacing)
on the other hand, sx= {{ marginTop: '20px' }} is not great cause you are assigning a hard coded value that would remain immutable to a design change (say you want to modify our spacing at the theme level, or to a "compact theme" etc)
Whenever possible, us sx and theme values, or theme.spacing(x) in styled components and makeStyles
There was a problem hiding this comment.
Understood. I usually pass the theme prop to sx to take utilize the spacing values like this:
sx={(theme) => ({ marginTop: theme.spacing(2) })}
However, in this specific case the theme is not available, so we can just use number values. Thanks for the explanation.
There was a problem hiding this comment.
I mean, theme is available if I want to pass it, it's more that I don't need it in this case since I am only worried about this particular CSS property. doing sx={(theme) => ({ marginTop: theme.spacing(x) })} would achieve the same thing and be just as right.
Description 📝
We had wrong label styles on the CreateCluster form, resulting in faux bolds, line height and letter spacing. Was very prominent on safari which has more font rendering fidelity than chrome.
Changes 🔄
Preview 📷
How to test 🧪
Prerequisites
Reproduction steps
Verification steps
As an Author I have considered 🤔
Check all that apply
Commit message and pull request title format standards
<commit type>: [JIRA-ticket-number] - <description>Commit Types:
feat: New feature for the user (not a part of the code, or ci, ...).fix: Bugfix for the user (not a fix to build something, ...).change: Modifying an existing visual UI instance. Such as a component or a feature.refactor: Restructuring existing code without changing its external behavior or visual UI. Typically to improve readability, maintainability, and performance.test: New tests or changes to existing tests. Does not change the production code.upcoming: A new feature that is in progress, not visible to users yet, and usually behind a feature flag.Example:
feat: [M3-1234] - Allow user to view their login history